[SPARK-47723][CORE][TESTS] Introduce a tool that can sort alphabetically enumeration field in `LogEntry` automatically by panbingkun · Pull Request #45867 · apache/spark

panbingkun · 2024-04-04T02:21:29Z

What changes were proposed in this pull request?

The pr aims to introduce a tool that can sort alphabetically enumeration field in LogEntry automatically.

Why are the changes needed?

Enable developers to more conveniently write the enumeration values in LogEntry in alphabetical order according to the requirements of structured log development documents.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manually test.

SPARK_GENERATE_GOLDEN_FILES=1 
build/sbt "common-utils/testOnly *LogKeySuite -- -t \"LogKey enumeration fields are correctly sorted\""

Pass GA.

Was this patch authored or co-authored using generative AI tooling?

No.

…lly enumeration field in `LogEntry` automatically

panbingkun · 2024-04-04T03:02:10Z

cc @gengliangwang @dtenedor @dongjoon-hyun

gengliangwang · 2024-04-04T04:14:21Z

+  test("LogKey enumeration fields are correctly sorted") {
+    val originalKeys = LogKey.values.toSeq
+    val sortedKeys = originalKeys.sortBy(_.toString)
+    if (regenerateGoldenFiles) {


Let's move the regeneration to a new method instead of keeping in the test case.

Do you mean: extract the regenerated method into an independent method and call it in that place?

spark/core/src/test/scala/org/apache/spark/SparkThrowableSuite.scala

Lines 90 to 117 in 3f6ac60

test("Error classes are correctly formatted") {

val errorClassFileContents =

IOUtils.toString(errorJsonFilePath.toUri.toURL.openStream(), StandardCharsets.UTF_8)

val mapper = JsonMapper.builder()

.addModule(DefaultScalaModule)

.enable(SerializationFeature.INDENT_OUTPUT)

.build()

val prettyPrinter = new DefaultPrettyPrinter()

.withArrayIndenter(DefaultIndenter.SYSTEM_LINEFEED_INSTANCE)

val rewrittenString = mapper.configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true)

.setSerializationInclusion(Include.NON_ABSENT)

.writer(prettyPrinter)

.writeValueAsString(errorReader.errorInfoMap)

if (regenerateGoldenFiles) {

if (rewrittenString.trim != errorClassFileContents.trim) {

val errorClassesFile = errorJsonFilePath.toFile

logInfo(s"Regenerating error class file $errorClassesFile")

Files.delete(errorClassesFile.toPath)

FileUtils.writeStringToFile(

errorClassesFile,

rewrittenString + lineSeparator,

StandardCharsets.UTF_8)

}

} else {

assert(rewrittenString.trim == errorClassFileContents.trim)

}

}

The basic idea is that when without setting environment variables SPARK_GENERATE_GOLDEN_FILES (default is false), the UT will only check whether the enumeration fields in LogKey have been sorted alphabetically.

The steps used in my process of migrating error classes are roughly as follows:

Before I submit the changes, if I remember the formatting step, I will use the following command to regenerate the error classes file error-classes.json and submit them.

SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "core/testOnly *SparkThrowableSuite -- -t \"Error classes are correctly formatted\""

If I forget this step, if there are incorrect item in error-classes.json that are not sorted alphabetically, GA will help me discover (UT failed), then I will execute above command on local and resubmit the changes.

I guess our scene is very similar to this.

Yeah, create a new method in this file .

Okay, let me do it right now.

gengliangwang · 2024-04-04T05:37:10Z

+  /* Used to regenerate the LogKey class file. Run:
+   {{{
+      SPARK_GENERATE_GOLDEN_FILES=1 build/sbt \
+        "common-utils/testOnly *LogKeySuite -- -t \"LogKey enumeration fields are correctly sorted\""


Shall we remove the -t ... from the instruction to make it simpler?

panbingkun · 2024-04-04T06:01:23Z

@gengliangwang all done.
Thanks.

Co-authored-by: Gengliang Wang <gengliang@apache.org>

gengliangwang · 2024-04-04T17:56:02Z

-    val keys = LogKey.values.toSeq
-    assert(keys === keys.sortBy(_.toString),
-      "LogKey enumeration fields must be sorted alphabetically")
+  // scalastyle:off line.size.limit


This is no longer needed.

gengliangwang · 2024-04-04T17:56:17Z

-    assert(keys === keys.sortBy(_.toString),
-      "LogKey enumeration fields must be sorted alphabetically")
+  // scalastyle:off line.size.limit
+  /* Used to regenerate the LogKey class file. Run:


Let's move this before line 32 class LogKeySuite

gengliangwang · 2024-04-05T00:04:39Z

Thanks, merging to master

[SPARK-47723][CORE][TESTS] Introduce a tool that can sort alphabetica…

3a25579

…lly enumeration field in `LogEntry` automatically

panbingkun changed the title ~~[SPARK-47723][CORE][TESTS] Introduce a tool that can sort alphabetically enumeration field in LogEntry automatically~~ [WIP][SPARK-47723][CORE][TESTS] Introduce a tool that can sort alphabetically enumeration field in LogEntry automatically Apr 4, 2024

fix conflict

215d6af

panbingkun changed the title ~~[WIP][SPARK-47723][CORE][TESTS] Introduce a tool that can sort alphabetically enumeration field in LogEntry automatically~~ [SPARK-47723][CORE][TESTS] Introduce a tool that can sort alphabetically enumeration field in LogEntry automatically Apr 4, 2024

panbingkun marked this pull request as ready for review April 4, 2024 03:01

gengliangwang reviewed Apr 4, 2024

View reviewed changes

panbingkun added 3 commits April 4, 2024 13:55

fix

e5411bd

Merge branch 'master' into SPARK-47723

b37e6aa

remove -- -t

81b1e65

gengliangwang reviewed Apr 4, 2024

View reviewed changes

Comment thread common/utils/src/test/scala/org/apache/spark/util/LogKeySuite.scala Outdated

update LogKeySuite.scala

aeba333

Co-authored-by: Gengliang Wang <gengliang@apache.org>

gengliangwang reviewed Apr 4, 2024

View reviewed changes

panbingkun added 2 commits April 5, 2024 06:05

Merge branch 'master' into SPARK-47723

dea198e

adjust position about comment

63bcd58

gengliangwang approved these changes Apr 5, 2024

View reviewed changes

gengliangwang closed this in fb96b1a Apr 5, 2024

	test("Error classes are correctly formatted") {
	val errorClassFileContents =
	IOUtils.toString(errorJsonFilePath.toUri.toURL.openStream(), StandardCharsets.UTF_8)
	val mapper = JsonMapper.builder()
	.addModule(DefaultScalaModule)
	.enable(SerializationFeature.INDENT_OUTPUT)
	.build()
	val prettyPrinter = new DefaultPrettyPrinter()
	.withArrayIndenter(DefaultIndenter.SYSTEM_LINEFEED_INSTANCE)
	val rewrittenString = mapper.configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true)
	.setSerializationInclusion(Include.NON_ABSENT)
	.writer(prettyPrinter)
	.writeValueAsString(errorReader.errorInfoMap)

	if (regenerateGoldenFiles) {
	if (rewrittenString.trim != errorClassFileContents.trim) {
	val errorClassesFile = errorJsonFilePath.toFile
	logInfo(s"Regenerating error class file $errorClassesFile")
	Files.delete(errorClassesFile.toPath)
	FileUtils.writeStringToFile(
	errorClassesFile,
	rewrittenString + lineSeparator,
	StandardCharsets.UTF_8)
	}
	} else {
	assert(rewrittenString.trim == errorClassFileContents.trim)
	}
	}

Uh oh!

Conversation

panbingkun commented Apr 4, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

panbingkun commented Apr 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panbingkun commented Apr 4, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gengliangwang commented Apr 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants